Skip to content

fix(runner): cap HuggingFace Retry-After backoff at 30s - #50

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-huggingface-retry-after-cap
Open

fix(runner): cap HuggingFace Retry-After backoff at 30s#50
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-huggingface-retry-after-cap

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

clawscan benchmark clawhub-security-signals fetches HuggingFace dataset rows and retries on 429/5xx. huggingFaceRowsBackoff honors Retry-After as integer seconds or an HTTP-date with no ceiling, while the quadratic fallback is already capped at 30s.

The public CLI builds HuggingFaceBenchmarkClient with a nil Context, so requestContext() is context.Background() and the retry select never expires. A 429 with Retry-After: 3600 blocks the process for one hour per attempt (up to five waits). This change caps Retry-After at the same 30s ceiling. It does not add a process-wide signal context.

The uncapped header path was introduced in #3 (2026-06-25). #46 made the wait cancelable when a context is set; it did not cap the delay.

Evidence

Red (unfixed huggingFaceRowsBackoff):

$ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
=== RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
    submission_test.go:276: Retry-After 3600 = 1h0m0s, want 30s
--- FAIL: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
FAIL
FAIL    github.com/openclaw/clawscan/internal/runner    0.248s
FAIL

Green (after the 30s cap):

$ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
=== RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
--- PASS: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
PASS
ok      github.com/openclaw/clawscan/internal/runner    0.263s

Retry-After: 3600 and an HTTP-date two hours ahead both return 30s. The quadratic fallback path is unchanged.

Real behavior proof

  • Behavior or issue addressed: HuggingFace row-fetch Retry-After delays (integer seconds and HTTP-date) are capped at 30s, matching the existing fallback ceiling, so a 429 cannot stall clawscan benchmark for an hour per attempt.

  • Real environment tested: macOS 26.6.2 (Darwin 25.6.0 arm64), go1.27.0 darwin/arm64, checkout /tmp/oc-pr-clawscan-F003 on fix/f003-huggingface-retry-after-cap.

  • Exact steps or command run after this patch:

    go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/

  • Evidence after fix: terminal output from the patched tree:

    $ go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/
=== RUN   TestHuggingFaceRowsBackoffCapsRetryAfter
--- PASS: TestHuggingFaceRowsBackoffCapsRetryAfter (0.00s)
PASS
ok      github.com/openclaw/clawscan/internal/runner    0.263s

  • Observed result after fix: Retry-After: 3600 is 30s (was 1h). An HTTP-date two hours ahead is also 30s. The helper no longer returns a multi-hour wait.

  • What was not tested: A live HuggingFace 429 with a long Retry-After header. Network fetch of datasets-server.huggingface.co during this change.

Command: go test -count=1 -timeout 30s -v -run TestHuggingFaceRowsBackoffCapsRetryAfter ./internal/runner/

Observed: unfixed delay was 1h0m0s for Retry-After: 3600; patched delay is 30s for both 3600 and a future HTTP-date.

Expected: both header forms cap at 30s.

Time: 11:07:34 PDT (2026-08-29 18:07:34 UTC)

Date: 2026-08-29

Environment: macOS 26.6.2, Darwin 25.6.0 arm64, go1.27.0 darwin/arm64

@SebTardif
fix(runner): cap HuggingFace Retry-After backoff at 30s
b7be605 
Honor Retry-After integer seconds and HTTP-date values only up to the
existing 30s quadratic fallback ceiling. A 429 with Retry-After: 3600
could block clawscan benchmark for an hour per attempt when the client
context is Background.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested review from a team and Patrick-Erichsen as code owners August 29, 2026 18:09
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 2:13 PM ET / 18:13 UTC.

ClawSweeper review

What this changes

The PR limits numeric and HTTP-date Hugging Face Retry-After delays to 30 seconds and adds helper tests for both forms.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 5 items remain

Keep this PR open: its 30-second cap is a focused availability improvement, but numeric Retry-After values can overflow before the new cap is applied, and the supplied proof only exercises the helper rather than the retrying HTTP path.

Priority: P1
Reviewed head: b7be605e23df7db4203400710d007f0a71c2cd2e

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is narrowly scoped, but its numeric ceiling is incomplete and its evidence does not exercise the production retry path.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed delay owner feeds fetchHuggingFaceRowsPage, which waits before retrying an HTTP request, but the supplied terminal trace calls only huggingFaceRowsBackoff directly. It does not demonstrate a real transport client receiving a long header and the observed retry outcome. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed delay owner feeds fetchHuggingFaceRowsPage, which waits before retrying an HTTP request, but the supplied terminal trace calls only huggingFaceRowsBackoff directly. It does not demonstrate a real transport client receiving a long header and the observed retry outcome. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Introduced cap applies after duration conversion: The PR changes the numeric header path to convert arbitrary parsed seconds to time.Duration before calling min, so values whose nanosecond conversion overflows do not reach the intended 30-second ceiling correctly.
Retry path consumes the computed delay: The production fetch loop passes response headers to the delay helper and waits before its next request, making the arithmetic part of benchmark availability behavior.
Current main does not contain the cap: Current main still returns an uncapped numeric or HTTP-date delay; the PR head is not an ancestor of fetched main.
Findings 1 actionable finding [P2] Clamp Retry-After seconds before converting to Duration
Security None None.

How this fits together

ClawScan’s benchmark runner fetches dataset rows from Hugging Face and retries transient HTTP failures. The computed retry delay controls when the benchmark client sends its next row request.

flowchart LR
A[Benchmark command] --> B[Hugging Face row client]
B --> C[HTTP response]
C --> D{Retriable status}
D -->|429 or 5xx| E[Retry delay policy]
E --> B
D -->|Success| F[Benchmark rows]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed delay owner feeds fetchHuggingFaceRowsPage, which waits before retrying an HTTP request, but the supplied terminal trace calls only huggingFaceRowsBackoff directly. It does not demonstrate a real transport client receiving a long header and the observed retry outcome. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Clamp Retry-After seconds before converting to Duration (P2) - The changed expression converts arbitrary seconds to nanoseconds before applying min. A parseable large value such as 10000000000 overflows to a negative duration, so the retry happens immediately instead of observing the 30-second ceiling. Bound seconds first and add coverage; this blocker was already visible at the same reviewed head.
  • Resolve merge risk (P1) - A very large numeric Retry-After can overflow during time.Duration conversion before min applies, producing a negative delay and bypassing the intended ceiling.
  • Resolve merge risk (P1) - The supplied terminal evidence tests the helper directly and does not show a real HTTP client receiving a long header and completing the production retry path.
  • Complete next step (P2) - A narrow mechanical correction can make the numeric cap overflow-safe, but contributor-supplied production-path proof remains required before merge.

Findings

  • [P2] Clamp Retry-After seconds before converting to Duration — internal/runner/benchmark.go:932
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +2/-2, tests +13, 2 files affected The small change is localized, but its tests currently validate only the delay helper rather than an end-to-end retry.

Merge-risk options

Maintainer options:

  1. Make numeric conversion overflow-safe (recommended)
    Compare parsed delta-seconds with the 30-second limit before converting them to a duration, add a large-value regression test, and refresh production-path proof.

Technical review

Best possible solution:

Clamp numeric seconds to the 30-second maximum before converting to time.Duration, cover that boundary, and provide a redacted production-path retry trace.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: on a 64-bit build, a numeric header such as 10000000000 overflows when converted to nanoseconds before min is evaluated, so it cannot reliably produce the required 30-second cap.

Is this the best way to solve the issue?

No: the HTTP-date cap is at the right boundary, but numeric seconds must be bounded before duration conversion so the cap applies to every parseable non-negative value.

Full review comments:

  • [P2] Clamp Retry-After seconds before converting to Duration — internal/runner/benchmark.go:932
    The changed expression converts arbitrary seconds to nanoseconds before applying min. A parseable large value such as 10000000000 overflows to a negative duration, so the retry happens immediately instead of observing the 30-second ceiling. Bound seconds first and add coverage; this blocker was already visible at the same reviewed head.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6432c40f2340.

Labels

Label changes:

  • remove merge-risk: 🚨 compatibility: Current PR review merge-risk labels are merge-risk: 🚨 availability.

Label justifications:

  • P1: An unbounded Hugging Face retry delay can make a benchmark invocation appear stalled for up to an hour per retry.
  • merge-risk: 🚨 availability: The new cap can be bypassed by numeric duration overflow, leaving the retry path capable of immediate repeated requests rather than the intended bounded wait.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed delay owner feeds fetchHuggingFaceRowsPage, which waits before retrying an HTTP request, but the supplied terminal trace calls only huggingFaceRowsBackoff directly. It does not demonstrate a real transport client receiving a long header and the observed retry outcome. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Acceptance criteria:

  • [P1] go test -count=1 ./internal/runner.
  • [P1] go test -count=1 ./...
  • [P1] go vet ./...

What I checked:

  • Introduced cap applies after duration conversion: The PR changes the numeric header path to convert arbitrary parsed seconds to time.Duration before calling min, so values whose nanosecond conversion overflows do not reach the intended 30-second ceiling correctly. (internal/runner/benchmark.go:932, b7be605e23df)
  • Retry path consumes the computed delay: The production fetch loop passes response headers to the delay helper and waits before its next request, making the arithmetic part of benchmark availability behavior. (internal/runner/benchmark.go:900, b7be605e23df)
  • Current main does not contain the cap: Current main still returns an uncapped numeric or HTTP-date delay; the PR head is not an ancestor of fetched main. (internal/runner/benchmark.go:932, 6432c40f2340)
  • Feature history routing: Merged retry work dates to the Hugging Face rate-limit change and was later updated for cancellation support; both remain relevant current-main history for this path. (internal/runner/benchmark.go:873, 113c1a8ede9f)
  • Prior blocker remains at the same head: The prior completed review identified duration-conversion clamping as a P2 concern at the exact current head SHA; no new head commit has addressed it. (internal/runner/benchmark.go:932, b7be605e23df)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Clamp parsed seconds before duration conversion and add a large numeric-header regression case.
  • Add redacted terminal proof using the real benchmark client and an injected long Retry-After response; update the PR body afterward to trigger re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (14 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-30T17:00:40.359Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Do not retry before a server-directed Retry-After expires | [P2] Clamp parsed seconds before converting to a duration
  • reviewed 2026-08-30T21:59:50.397Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor server-directed Retry-After before retrying | [P2] Clamp seconds before converting to time.Duration
  • reviewed 2026-08-31T02:04:54.189Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Do not retry before a server-directed Retry-After expires | [P2] Clamp Retry-After seconds before duration conversion
  • reviewed 2026-08-31T07:13:47.659Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor server-provided Retry-After values | [P2] Clamp numeric seconds before converting duration
  • reviewed 2026-08-31T11:07:23.795Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server Retry-After delays | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-01T04:22:34.187Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server Retry-After delays | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-01T07:14:41.612Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server-directed Retry-After delays | [P2] Clamp numeric Retry-After before duration conversion
  • reviewed 2026-09-01T10:58:37.796Z sha b7be605 :: needs real behavior proof before merge. :: [P1] Honor valid server Retry-After values | [P2] Clamp seconds before duration conversion

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Patrick-Erichsen Patrick-Erichsen Awaiting requested review from Patrick-Erichsen Patrick-Erichsen is a code owner

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SebTardif